feat: Add Raw SQL Chart support to external dashboard APIs - #1877

🦋 Changeset detected

Latest commit: 804a484

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	Mar 11, 2026 1:31pm

Code Review

PR: feat: Add Raw SQL Chart support to external dashboard APIs

• 🔒 Missing connectionId ownership validation -> The getMissingSources function in dashboards.ts:36 only validates sourceIds for builder configs, but there is no equivalent check to verify that the connectionId in a raw SQL chart config belongs to the requesting team. A user could reference another team's connection ID. Add a getMissingConnections check parallel to getMissingSources, called in both POST (~line 1516) and PUT (~line 1739) handlers.

• Warning: externalDashboardOtherRawSqlChartConfigSchema groups table/number/pie -> These three display types are grouped into one schema (zod.ts:1078), but the OpenAPI spec defines them as separate schemas (TableRawSqlChartConfig, NumberRawSqlChartConfig, PieRawSqlChartConfig). The union type allows e.g. fillNulls on a number chart, which is not reflected in the API docs. Consider splitting into per-display-type schemas or noting the mismatch.

• Warning: Removed discriminator from TileConfig oneOf -> The discriminator was removed from both the OpenAPI spec and inline JSDoc. Without it, code generation tools cannot efficiently determine which schema to use. Consider adding a composite discriminator or noting this as a known limitation.

Conversion logic to/from RawSqlSavedChartConfig looks correct, type safety is well-enforced with satisfies, and test coverage is thorough.

PR Review: feat: Add Raw SQL Chart support to external dashboard APIs

• ⚠️ Misleading error log message in convertToExternalTileChartConfig (utils/dashboards.ts) for Search/Markdown/Heatmap raw SQL cases: message says 'raw SQL charts should have display type "sql"' but displayType is never "sql" — that's configType. Should say something like 'unsupported display type for raw SQL chart'.

• ⚠️ z.discriminatedUnion → z.union: The change is architecturally necessary given dual discriminants, but Zod will now try all schemas in order on every validation, producing generic union errors instead of targeted ones when invalid input is sent. Consider documenting this trade-off or using a custom pre-check to route to the right sub-union (e.g., check configType === 'sql' first).

• ✅ Multi-tenancy is correctly enforced: getMissingConnections validates that all connectionIds belong to the requesting team before accepting the dashboard.

• ✅ Round-trip tests for all 5 raw SQL chart types are present and comprehensive.

PR Review: feat: Add Raw SQL Chart support to external dashboard APIs

Overall the implementation is solid — correct ownership validation, good test coverage, clean separation of builder vs. raw-SQL paths. A couple of items worth addressing:

• ⚠️ OpenAPI discriminator removed — Removing the discriminator block from TileConfig is a potentially breaking change for API clients that use code generators (e.g. openapi-generator, swagger-codegen). Since the schema now requires two fields to discriminate (configType + displayType), a single discriminator can't cover both, but clients relying on the old discriminator mapping will break. Consider calling this out explicitly in the changeset or PR description.

• ⚠️ Double-parse pattern in zod.ts — silent 500 risk — In externalDashboardTileConfigSchema, the .superRefine() forwards issues via ctx.addIssue() (without fatal: true), then .transform() calls schema.parse(data). In Zod v3, a non-fatal issue in superRefine does not prevent the transform from running, so if superRefine marks a field invalid but doesn't abort the parse, schema.parse(data) in the transform will throw an uncaught ZodError → 500 instead of 400. Fix: add { fatal: true } to each ctx.addIssue call in the superRefine block, or guard the transform with an early return on failed validation.

• ℹ️ sqlTemplate accepts empty strings — No minimum length is enforced on the sqlTemplate field (only maxLength: 100000). An empty template will pass validation but likely fail at query execution time. Consider z.string().min(1).max(100000) to surface the error earlier.

✅ Security check (cross-team connection ownership via getMissingConnections) is correctly implemented and tested.

PR Review: feat: Add Raw SQL Chart support to external dashboard APIs

✅ No critical issues found.

The implementation is solid:

• Multi-tenancy enforced correctly: getMissingConnections validates connection IDs belong to the requesting team before saving, with tests for the cross-team rejection case
• fillNulls round-trips correctly: true→stores undefined→reads back as true (mirrors existing builder pattern)
• Type exhaustiveness is maintained via satisfies checks and externalConfig satisfies never guards
• Zod validation properly routes to the correct sub-schema via configType discriminant and strips unknown fields in the transform step

One minor note: the getMissingConnections function fetches all team connections to validate IDs (same pattern as getMissingSources), which is fine at current scale.

🤖 Generated with Claude Code

E2E Test Results

✅ All tests passed • 88 passed • 3 skipped • 892s

Tests ran across 4 shards in parallel.

View full report →

PR Review

• ⚠️ Duplicated routing logic in zod.ts: The configType === 'sql' branching is copy-pasted into both superRefine and transform (lines ~2137-2143 and ~2165-2171). If the routing condition changes in one place but not the other, validation and stripping will silently diverge → extract into a shared helper getRawSqlSchema(data).

• ⚠️ getMissingConnections fetches all team connections: getConnectionsByTeam(team) loads the full connection list then filters in memory. For teams with many connections this is unnecessary — a targeted Connection.find({ team, _id: { $in: [...connectionIds] } }) query would be more efficient and avoid the full collection scan on every POST/PUT.

• ℹ️ stacked_bar raw SQL drops fillNulls in output conversion: In convertToExternalTileChartConfig, the StackedBar raw SQL branch sets fillNulls: config.fillNulls !== false, but the round-trip test for barRawSql sets fillNulls: false and expects it back — verify this is actually preserved correctly end-to-end (the false sentinel is fillNulls === false ? false : undefined, which returns undefined when fillNulls is true, potentially losing the explicit true value in existing data).

✅ Security looks solid: cross-team connectionId ownership is validated on both POST and PUT. Alert deletion on tile-to-raw-SQL conversion is handled correctly with good test coverage.